-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: pass libp2pOptions to the bundle function #2591
Conversation
This allows bundle creation by merging/overriding option defaults instead of creating them in their entirety from scratch. e.g. ```js const ipfs = await IPFS.create({ libp2p: ({ libp2pOptions, peerInfo }) => { libp2pOptions.transports.push(...) return new Libp2p(defaultOptions) } }) ``` License: MIT Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
const libp2pOptions = getLibp2pOptions({ options, config, datastore, peerInfo, peerBook }) | ||
let libp2p | ||
|
||
if (typeof options.libp2p === 'function') { | ||
libp2p = options.libp2p({ libp2pOptions, options, config, datastore, peerInfo, peerBook }) | ||
} else { | ||
// Required inline to reduce startup time | ||
const Libp2p = require('libp2p') | ||
libp2p = new Libp2p(mergeOptions(libp2pOptions, get(options, 'libp2p', {}))) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, options is getting passed into getLibp2pOptions
, but then it is still passed into libp2p creation. Should getLibp2pOptions
just handle merging all of that so options doesn't need to be passed again?
options.libp2p({ libp2pOptions, options, config, datastore, peerInfo, peerBook })
Along those lines, why is options
being passed here? It feels like the result of getLibp2pOptions
should just be able to get passed directly to both new Libp2p
and options.libp2p
. No other params should be needed right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should getLibp2pOptions just handle merging all of that so options doesn't need to be passed again?
There may be some other IPFS option you want to use in your custom bundle function. It's "passed again" because that's what currently happens, and I'm trying to avoid a breaking change.
options.libp2p({ libp2pOptions, options, config, datastore, peerInfo, peerBook })
Along those lines, why is
options
being passed here? It feels like the result ofgetLibp2pOptions
should just be able to get passed directly to bothnew Libp2p
andoptions.libp2p
. No other params should be needed right?
options
is IPFS options, passed to the IPFS constructor. It's passed here because that's currently what happens when you provide a custom bundle function. If we removed it, we'd have a breaking change.
getLibp2pOptions
is passed IPFS options
and returns libp2p options (I would call them defaults, but they're not really because they're based off the IPFS options
and the IPFS config from the repo).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so options
and config
are for IPFS? If so, would it be easier to understand if in the scope of the custom bundler options
were the libp2p options and the IPFS ones were prefixed? If those are going to be exposed I think it might be helpful to make that distinction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm, that would be a breaking change. I think that would be clearer, but since the goal is to avoid the breaking change I think this is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would be clearer
FWIW I agree!
This is PoC use of ipfs/js-ipfs#2591
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a simple man, I see change that lets me remove a lot of code, I 👍
Check ipfs/ipfs-companion#811 for how this PR simplifies customizing libp2p for js-ipfs in Brave.
// Merge defaults with Node.js/browser/other environments options and configuration | ||
return mergeOptions( | ||
libp2pDefaults, | ||
getEnvLibp2pOptions({ options, config, datastore, peerInfo, peerBook }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: easier to read if options-to-be-merged follow same naming convention (also, runtime
is the name of directory, so let's reuse that name)
getEnvLibp2pOptions({ options, config, datastore, peerInfo, peerBook }), | |
libp2pRuntimeDefaults({ options, config, datastore, peerInfo, peerBook }), |
This is PoC use of ipfs/js-ipfs#2591 BREAKING CHANGE: switched to Async Iterators version of JS API https://blog.ipfs.io/2020-02-01-async-await-refactor/ Code that is not related to embedded node with chromesockets will be refactored in separate commit.
Switches JS API to async iterators where possible. Includes switch to libp2p config override via ipfs/js-ipfs#2591 BREAKING CHANGE: switched to Async Iterators version of JS API https://blog.ipfs.io/2020-02-01-async-await-refactor/
Switches JS API to async iterators where possible. Includes switch to libp2p config override via ipfs/js-ipfs#2591 BREAKING CHANGE: switched to Async Iterators version of JS API https://blog.ipfs.io/2020-02-01-async-await-refactor/
Switches JS API to async iterators where possible. Includes switch to libp2p config override via ipfs/js-ipfs#2591 BREAKING CHANGE: switched to Async Iterators version of JS API https://blog.ipfs.io/2020-02-01-async-await-refactor/
This allows bundle creation by merging/overriding option defaults instead of creating them in their entirety from scratch. This is for convenience.
The
libp2pOptions
passed to the bundle function include any options defined in the required runtime files (for Node.js/browser config).To allow this, there is a slight change to
runtime/libp2p-nodejs.js
andruntime/libp2p-browser.js
to return their options instead of a libp2p instance. This actually removes some repeated boilerplate.e.g.
resolves #2579